Skip to content

Support overloaded computed column functions - #2997

Open
GeertJohan wants to merge 1 commit into
graphile:mainfrom
GeertJohan:allow-overloaded-computed-functions
Open

Support overloaded computed column functions#2997
GeertJohan wants to merge 1 commit into
graphile:mainfrom
GeertJohan:allow-overloaded-computed-functions

Conversation

@GeertJohan

@GeertJohan GeertJohan commented Mar 16, 2026 •
edited
Loading

Copy link
Copy Markdown

Description

Support overloaded computed column functions, where the first argument is a table.
Fixes #2972
Adds a test for this specific scenario.

I deliberately did not modify the PgV4BehaviorPlugin as I suspect users will not appreciate a sudden change in behavior. So this still requires smart tags, just like computed column functions that are NOT overloaded.

AI disclaimer: I used Claude to investigate the codebase and implement parts of this PR, but am actively trying to avoid slop. I personally read all the changes line-by-line, except for the snapshots, and validate the changes to the best of my ability.

Regardless of the use of AI; this is my first PR on this project and first experience with the inner workings of graphile. Although I've been a user for a couple of years now and wrote some custom plugins, diving into a large project like this one always a little overwhelming at first. So please consider carefully and do not hesitate to just close my PR if it's bad, I'll understand.

About the snapshots, I'm not sure if all those changes are correct, this is what recreating the snapshots resulted in but I wonder if it's supposed to do that? It feels like the diff is way too large... I am not familiar enough (yet) with this project to asses this; help needed.

Since procedures need a unique name it probably makes sense to add the table the overloaded procedure is targeting.
For FUNCTION code(finance.invoices) we could go with either:

  • code_finance_invoices or
  • finance_invoices_code

I decided to go with the first since it has the same order of identifiers as the function definition (a(b.c) > a_b_c), and less likely to be confused/collide with the finance.invoices_code(..) approach of v4.

Performance impact

Should be relatively low. I used manual loops instead of functional to keep allocations low.

Security impact

unknown

Checklist

  • My code matches the project's code style and yarn lint:fix passes.
  • I've added tests for the new feature, and yarn test passes.
  • I have detailed the new feature in the relevant documentation.
    • Is dont think this is needed? This PR changes make that overloaded functions work the same as non-overloaded functions. It doesn't 'add' a new feature in that sense.
  • I have added this feature to 'Pending' in the RELEASE_NOTES.md file (if one exists).
    • Does not exist.
  • If this is a breaking change I've explained why.
    • Afaik not a breaking change, this only extends what was already possible with one computed column function. Users must explicitly +typeField on overloaded functions to use this change.

@github-project-automation github-project-automation Bot moved this to 馃尦 Triage in V5.0.0 Mar 16, 2026
@changeset-bot

changeset-bot Bot commented Mar 16, 2026 •
edited
Loading

Copy link
Copy Markdown

馃 Changeset detected

Latest commit: 740d7f9

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages
Name Type
graphile-build-pg Patch
postgraphile Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

benjie
benjie requested changes Mar 17, 2026
View reviewed changes

@benjie benjie left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the contribution; here's some guidance on how to help move this forward. I think we can end up with a much smaller PR that still achieves your goals 馃

Comment thread graphile-build/graphile-build-pg/src/plugins/PgProceduresPlugin.ts Outdated
Comment thread graphile-build/graphile-build-pg/src/plugins/PgProceduresPlugin.ts Outdated
Comment thread graphile-build/graphile-build-pg/src/plugins/PgProceduresPlugin.ts Outdated
Comment thread postgraphile/postgraphile/__tests__/kitchen-sink-schema.sql Outdated
@github-project-automation github-project-automation Bot moved this from 馃尦 Triage to 馃尡 In Progress in V5.0.0 Mar 17, 2026
@GeertJohan

GeertJohan commented Mar 22, 2026

Copy link
Copy Markdown
Author

I have pushed changes to address a number of comments. It also moves towards the consistent prefix behavior, but that does break existing functions (see the thread).

In general I'm strictly against breaking changes, however since we are still pre 5.0 stable and since it makes behavior more consistent across the board, I think it may be a good direction. The breaking change will probably have a very limited impact since it's a rare feature?

@GeertJohan

GeertJohan commented Mar 22, 2026

Copy link
Copy Markdown
Author

As a result of these changes, the query_compound_type_array is now renamed to compound_type_query_compound_type_array. This is the breaking change we're talking about.

I've made the test succeed now. But if we think this is the good way forward then it may make sense to rename query_compound_type_array to query_array (which is then renamed to compound_type_query_array to make the tests a little easier to read.

@GeertJohan
GeertJohan force-pushed the allow-overloaded-computed-functions branch from c731613 to 090c885 Compare March 23, 2026 07:24
benjie
benjie reviewed Mar 25, 2026
View reviewed changes

@benjie benjie left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the patience whilst we were working through the V5 release!

Comment thread graphile-build/graphile-build-pg/src/plugins/PgProceduresPlugin.ts Outdated
@benjie benjie removed this from V5.0.0 Apr 27, 2026
@benjie benjie added this to V5.X Apr 27, 2026
@github-project-automation github-project-automation Bot moved this from 馃尦 Triage to 馃尡 In Progress in V5.X Apr 27, 2026
@github-project-automation github-project-automation Bot moved this to 馃尦 Triage in V5.X Apr 27, 2026
@GeertJohan
GeertJohan force-pushed the allow-overloaded-computed-functions branch from 88d18cc to e8197a2 Compare July 14, 2026 19:35
benjie
benjie reviewed Jul 15, 2026
View reviewed changes

@benjie benjie left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not the simplest of problems to solve 馃槄

Let me know what you think of the ideas below.

Comment thread graphile-build/graphile-build-pg/src/plugins/PgProceduresPlugin.ts Outdated
Comment thread graphile-build/graphile-build-pg/src/plugins/PgProceduresPlugin.ts Outdated
Comment thread graphile-build/graphile-build-pg/src/plugins/PgProceduresPlugin.ts Outdated
Comment thread graphile-build/graphile-build-pg/src/plugins/PgProceduresPlugin.ts Outdated
@GeertJohan
GeertJohan force-pushed the allow-overloaded-computed-functions branch from 02b3e40 to 56bb459 Compare July 25, 2026 10:29
@GeertJohan
Add opt-in PgFunctionOverloadsPreset to support overloaded functions
740d7f9 
By default, PostGraphile skips overloaded functions because their
derived resource names would clash. This is now detected by comparing
inflected resource names, and a warning is logged when the skipped
overloads look like computed columns targeting different tables.

The new preset factors the input argument types into function resource
names (code(pets) becomes code__pets) so that overloads receive
distinct names. This enables overloaded computed column functions
targeting different tables, including across schemas.
@GeertJohan
GeertJohan force-pushed the allow-overloaded-computed-functions branch from 56bb459 to 740d7f9 Compare July 25, 2026 10:30
@GeertJohan

GeertJohan commented Jul 25, 2026 •
edited
Loading

Copy link
Copy Markdown
Author

Hi @benjie, I re-did the feature based on your feedback and based on the latest main (this branch was 474 commit behind 馃槄 ).

AI Disclaimer: I used Claude Fable 5 while making these changes.

@GeertJohan
GeertJohan requested a review from benjie July 25, 2026 16:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@benjie benjie Awaiting requested review from benjie

Requested changes must be addressed to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

V5.X
Status: 馃尡 In Progress

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Functions with same name but different parameter types are silently dropped (overload detection too aggressive)

2 participants

@GeertJohan @benjie